-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ui: Auth Methods List view #9617
Conversation
cda9c88
to
00132bf
Compare
00132bf
to
91d946e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think code-wise this looks great. I noticed a few naming things which would be good to fix up, and while I was doing that I noticed that we might not be mocking the API responses correctly here? The one that is most important is the MaxTokenTTL
property, as if that is not in the list responses, then we won't be able to show that value in our listings.
ui/packages/consul-ui/app/components/consul/auth-method/list/index.hbs
Outdated
Show resolved
Hide resolved
ui/packages/consul-ui/app/components/consul/auth-method/list/pageobject.js
Outdated
Show resolved
Hide resolved
ui/packages/consul-ui/tests/acceptance/dc/acls/auth-methods/sorting.feature
Show resolved
Hide resolved
ui/packages/consul-ui/app/components/consul/auth-method/search-bar/index.hbs
Outdated
Show resolved
Hide resolved
75873ab
to
18584d2
Compare
18584d2
to
8bec483
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on all this 👍 , I left a few nits but nothing major, mainly word-y things.
ui/packages/consul-ui/app/components/consul/auth-method/search-bar/index.hbs
Outdated
Show resolved
Hide resolved
ui/packages/consul-ui/app/components/consul/auth-method/type/index.hbs
Outdated
Show resolved
Hide resolved
oidc: (item, value) => item.Type === value, | ||
}, | ||
source: { | ||
local: (item, value) => item.TokenLocality === value || item.TokenLocality === '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we figure out if Consul ever responds with an empty value for TokenLocality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it responds with empty ''
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that the mocks where you see that empty value or consul itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, right. I need to verify this with BE dev.
ui/packages/consul-ui/tests/acceptance/dc/acls/auth-methods/sorting.feature
Outdated
Show resolved
Hide resolved
ui/packages/consul-ui/app/components/consul/auth-method/index.scss
Outdated
Show resolved
Hide resolved
ui/packages/consul-ui/app/components/consul/auth-method/search-bar/index.hbs
Outdated
Show resolved
Hide resolved
ui/packages/consul-ui/app/components/consul/auth-method/search-bar/index.hbs
Outdated
Show resolved
Hide resolved
ui/packages/consul-ui/app/components/consul/auth-method/index.scss
Outdated
Show resolved
Hide resolved
ui/packages/consul-ui/app/components/consul/auth-method/search-bar/index.hbs
Outdated
Show resolved
Hide resolved
ui/packages/consul-ui/app/components/consul/auth-method/search-bar/index.hbs
Show resolved
Hide resolved
ui/packages/consul-ui/app/components/consul/auth-method/search-bar/index.hbs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff! LGTM!
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/328626. |
During #9617 we added a list view only for AuthMethods, but not a detail view. We did add the Adapter/Serializer that collected/reshaped data for a detail view. The test for this serializer was skipped here, but I'm not sure why. We then added #9845 which began to use this AuthMethod Serializer, but we didn't go back to finish up the skipped test here either. This PR unskips this test and finishes off the test correctly.
During #9617 we added a list view only for AuthMethods, but not a detail view. We did add the Adapter/Serializer that collected/reshaped data for a detail view. The test for this serializer was skipped here, but I'm not sure why. We then added #9845 which began to use this AuthMethod Serializer, but we didn't go back to finish up the skipped test here either. This PR unskips this test and finishes off the test correctly.
/acls/auth-method/:id
and/acls/auth-methods
endpoints